Swaps raw pointer types with NonNull for better safety and niches by ErisianArchitect · Pull Request #687 · TheDan64/inkwell

ErisianArchitect · 2026-04-18T06:59:19Z

Description

Many of the types used LLVM*Ref types, which are type aliases for *mut LLVM*. These pointers are null-checked before creating new instances of these types, so I thought it would make sense to use NonNull instead so that there would be better safety guarantees, and it would enable these types to have at least one niche, allowing for this condition: size_of::<T>() == size_of::<Option<T>>() && align_of::<T>() == align_of::<Option<T>>().

Related Issue

Closes #686

How This Has Been Tested

I finally figured out clippy, so I have a bash function that I run that runs clippy, runs all the tests, then runs cargo fmt. So all checks should hopefully pass on the first try. Fingers crossed.

It's quite a big update. It was extremely repetitive and tedious, but I think I got everything.

Option<Breaking Changes>

I'm pretty sure there are no breaking changes.

Checklist

I have read the Contributing Guide

… least one niche.

ErisianArchitect · 2026-04-18T07:17:39Z

Third times the charm, I guess.

ErisianArchitect · 2026-04-18T07:19:31Z

Oh, I forgot to mention, I also added a couple functions to support.

const_assert and assert_niche.

assert_niche is used to assert that types have a niche (same size and alignment as Option<T>).

const_assert is just used by assert_niche, but it could be used anywhere that a compile time assertions needs to be done.

ErisianArchitect · 2026-04-18T07:44:12Z

+#[repr(transparent)]
 #[derive(Debug)]
 pub struct OperandBundle<'ctx> {
-    bundle: Cell<LLVMOperandBundleRef>,


As far as I could tell, bundle did not need to be a Cell, so I changed that too.

ErisianArchitect · 2026-04-18T08:11:44Z

-        unsafe { *self.0.get_or_insert_with(|| LLVMCreateTargetMachineOptions()) }
+        unsafe {
+            self.0
+                .get_or_insert_with(|| NonNull::new_unchecked(LLVMCreateTargetMachineOptions()))


This is honestly a little bit weird, but it works.

Do you think The struct should just be initialized with LLVMCreateTargetMachineOptions? Or does it being Option make sense?

I'm not sure. I can look into it. I'm not sure why it's an Option in the first place, I didn't want to focus on anything besides what I was working on. I didn't want to give you too much to review. I think the reason it's an Option is because it's nullable. But I'll find out for sure and get back to you.

/// LLVM target machine options provide another way to create target machines, /// used with [Target::create_target_machine_from_options]. /// /// The option structure exposes an additional setting (i.e., the target ABI) /// and provides default values for unspecified settings. #[llvm_versions(18..)] #[derive(Default, Debug)] pub struct TargetMachineOptions(Option<NonNull<LLVMOpaqueTargetMachineOptions>>);

I guess I didn't have to look far after all. I'm not entirely sure what this documentation is saying, but I'll figure it out. Maybe it doesn't need to be an Option, maybe it does. But I think it's a good idea to figure out that reason so we can include that information with the documentation.

Okay, I'm back again. From the looks of it, it seems that the reason that it's an Option is for some sort of lazy initialization. I'm not sure why...

[After some more digging while writing this comment]

I narrowed it down to this commit: https://github.com/TheDan64/inkwell/pull/483/changes#diff-05702e78430359853e5b38404035fa394540368bee185a2cdc936a62b9cd65b0R1493

Unfortunately, he doesn't explain his changes.

But looking at the code, there doesn't appear to be any reason that TargetMachineOptions needs lazy initialization. I can remove it if you want.

I asked the robot just for the hell of it, and the robot claims that LLVMTargetMachineOptionsRef should never be null, so maybe it doesn't represent a default value of any sort. I'll look deeper, because I don't trust anything the robot says.

Could you please open up a follow up issue? You're right that this PR is already big enough as is. Was mostly curious, it doesnt need immediate attention

Sure, I can do that.

ErisianArchitect

I don't know if it counts for much, but I read through the entire codebase at least twice while working on this PR, and have now read through all of my changes.

ErisianArchitect · 2026-04-18T08:16:33Z

Oh, and one last note: I didn't apply this optimization to passes.rs. It's not that I couldn't have for some reason, it's just that I'd already read through all the code twice by the time I got to that, and I didn't want to go back and read it again (because of all the conditionally compiled blocks that don't show errors in my environment). I did the best that I could. Considering all the checks passed, I'm hoping that I didn't make any mistakes. I hope this optimization is worth the time I just spent on this, because it was a lot more work than I thought it was going to be.

TheDan64 · 2026-04-20T21:12:22Z

Looks cool, it'll take me a while to review it all

TheDan64 · 2026-04-20T21:14:59Z

+#[inline(always)]
+pub(crate) const fn const_assert(assertion: bool) {
+    if !assertion {
+        panic!("Assertion Failed");


Is it possible to be able to take a static string? Would be nice if the panic had more specific details

Unfortunately, no, not at compile time. panic expects a format argument, and format args, but it can't perform formatting at compile time.

The best recommendation is to include a comment with the assertion. It's marked with #[track_caller], so the error will appear at the location of the assertion.

ErisianArchitect · 2026-04-20T21:22:37Z

Looks cool, it'll take me a while to review it all

Yeah, sorry about that, haha. I really did not expect this change to affect this many lines.

ErisianArchitect added 3 commits April 17, 2026 23:52

Swaps raw pointer types with NonNull for better safety and to have at…

d25e84b

… least one niche.

Fixed clippy problems. Hopefully.

0ec97f8

cargo fmt

37be92f

ErisianArchitect added 2 commits April 18, 2026 00:22

Added alignment check to assert_niche.

f6b24e0

Added missing assertion.

703c2c9

ErisianArchitect commented Apr 18, 2026

View reviewed changes

TheDan64 self-requested a review April 20, 2026 20:57

TheDan64 assigned ErisianArchitect Apr 20, 2026

TheDan64 reviewed Apr 20, 2026

View reviewed changes

Comment thread src/support/mod.rs

TheDan64 reviewed Apr 20, 2026

View reviewed changes

TheDan64 reviewed Apr 21, 2026

View reviewed changes

Comment thread src/data_layout.rs

TheDan64 reviewed Apr 22, 2026

View reviewed changes

Comment thread src/attributes.rs

TheDan64 reviewed Apr 22, 2026

View reviewed changes

Comment thread src/builder.rs

Conversation

ErisianArchitect commented Apr 18, 2026

Description

Related Issue

How This Has Been Tested

Option<Breaking Changes>

Checklist

Uh oh!

ErisianArchitect commented Apr 18, 2026

Uh oh!

ErisianArchitect commented Apr 18, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ErisianArchitect left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ErisianArchitect commented Apr 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TheDan64 commented Apr 20, 2026

Uh oh!

Uh oh!

TheDan64 Apr 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ErisianArchitect commented Apr 20, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ErisianArchitect left a comment •

edited

Loading

ErisianArchitect commented Apr 18, 2026 •

edited

Loading

TheDan64 Apr 20, 2026 •

edited

Loading